Consolidate v1 MCP client utilities - #2467
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9f3f24d00
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This cross-cutting refactor changes shared MCP client infrastructure and launch behavior across several production harnesses, including Null/Bash mode selection, generated-script source injection, and NeMo Gym timeout handling. The scope and runtime reach warrant human review despite most helper logic being consolidated from existing implementations. You can add or adjust custom eligibility rules. Learn more. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 798ea7cda8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Overview
Consolidates v1 MCP client utilities and standalone harness code behind shared owning modules.
Details
Related work
Closes #2202
Note
Medium Risk
Touches eval harness launch paths and MCP tool wiring across Null, Bash, Browser Use, and NeMo Gym; behavioral differences between Null and Bash modes in the unified program warrant careful regression testing.
Overview
Moves duplicated MCP HTTP client, retry, tool discovery, and call helpers into
verifiers.v1.mcp.client, re-exported fromverifiers.v1.mcp, and wires NeMo Gym’s upstream MCP calls through that sharedmcp_clientinstead of a local session helper.Adds
standalone.pywithlaunch_chat_program(shared CLI/MCP/initial-messages launch path) andinline_mcp_client(splices the client module source into PEP 723 sandbox scripts). Null and Bash now shareminimal/program.py(Bash passes--bashplus edit/search/interception flags);null/program.pyis removed. Browser Use drops its inlined MCP copy and uses the same embed +launch_chat_programpattern.The shared minimal program keeps Null-specific behavior when
--bashis off (60s MCP enumeration cap, graceful exit on context-overflow API errors) and Bash-specific behavior when on (longer HTTP timeouts, unbounded MCP connect wait, local tools gated by flags).Reviewed by Cursor Bugbot for commit 1944f34. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Consolidate v1 MCP client utilities into shared
client.pyandlaunch_chat_programmcp_client,with_retry,connect_mcp,mcp_content_to_chat_content, andcall_mcphelpers, replacing per-harness duplicatesstandalone.launch_chat_programandinline_mcp_clientin standalone.py to centralize wire-argument passing and embed the MCP client into PEP 723 scripts at build timelaunch_chat_programwith the sharedminimal.PROGRAM_SOURCE; deletes the standalone null programminimal/program.pyto serve both Null and Bash harnesses via a--bashflag that enables bash tooling, unbounded MCP enumeration, and longer model timeout; without it, bash tooling is disabled and context-overflow errors terminate cleanlyNeMoGymToolset.list_tools/call_toolin toolset.py to use the sharedmcp_clienthelper with explicit timeoutsminimal.programnow relies on inlined shared functions (mcp_client,with_retry,connect_mcp,call_mcp,mcp_content_to_chat_content) being present at runtime viainline_mcp_client; ifPROGRAM_SOURCEis constructed without callinginline_mcp_client, the script will fail withNameErrorMacroscope summarized 1944f34.